Added Validator to WarpSyncProvider by Klapeyron · Pull Request #10223 · paritytech/polkadot-sdk

Klapeyron · 2025-11-06T11:22:45Z

Description

It looks like WarpSyncProvider in sc-network-sync has direct dependency to sp-consensus-gradnpa types, specially SetId and AuthorityList:

/// Warp sync backend. Handles retrieving and verifying warp sync proofs.
pub trait WarpSyncProvider<Block: BlockT>: Send + Sync {
	/// Generate proof starting at given block hash. The proof is accumulated until maximum proof
	/// size is reached.
	fn generate(
		&self,
		start: Block::Hash,
	) -> Result<EncodedProof, Box<dyn std::error::Error + Send + Sync>>;
	/// Verify warp proof against current set of authorities.
	fn verify(
		&self,
		proof: &EncodedProof,
		set_id: SetId,
		authorities: AuthorityList,
	) -> Result<VerificationResult<Block>, Box<dyn std::error::Error + Send + Sync>>;
	/// Get current list of authorities. This is supposed to be genesis authorities when starting
	/// sync.
	fn current_authorities(&self) -> AuthorityList;
}

This PR is removing this dependency and replaces it with Validator, which can be used to validate proofs.

bkchr

Thank you for the pull request!

Sorry for the delay until you got this review. See my comment on what needs to be changed. The general idea of the pull request is fine, but the implementation needs to be changed a little bit.

bkchr · 2025-12-03T14:37:36Z

 }

 /// Warp sync backend. Handles retrieving and verifying warp sync proofs.
 pub trait WarpSyncProvider<Block: BlockT>: Send + Sync {


We should not introduce the Context type. This requires too many changes that are not really required.

I think we should change the WarpSyncProvider trait in the following way:

Remove verify and current_context.

Introduce a new create_verifier() -> Arc<Verifier> (maybe you come up with a better name).

trait Verifier<Block> { fn verify(proof) -> Result; /// Returns the context in which the verifier operates fn context() -> Block::Hash; }

context() for now will always return the genesis hash.

last_hash can then replaced with calls to context().

This way we don't require the pass through of the generic parameter to everywhere and also prepare the trait for the future.

This reverts commit efba492.

This reverts commit 2ef715e.

Klapeyron · 2025-12-04T18:53:57Z

 pub struct WarpSync<B: BlockT, Client> {
 	phase: Phase<B>,
-	client: Arc<Client>,
+	_client: Arc<Client>,


after lates changes, this client is only used in new function, but I left it to not adjust the interfaces elsewhere. Please let me know if we want to provide some phantom here, remove it or leave or something else

Let's remove it. We still need client in new. So, we don't need to change any interfaces, but we don't need to store it here.

Klapeyron · 2025-12-04T18:59:59Z

Thank you @bkchr! It simplified this code significantly, I will appreciate if you will allocate some time to make a second round of review, meanwhile I will prepare some prdoc.

Klapeyron · 2025-12-04T19:06:56Z

 			.ok_or_else(|| "Empty proof".to_string())?;
-		let (next_set_id, next_authorities) =
-			proof.verify(set_id, authorities, &self.hard_forks).map_err(Box::new)?;
+		let (_next_set_id, _next_authorities) = proof


here is a interior mutability case that is still not solved, I will take a look later

I am also curious how the CI tests passed, looks like none of the tests is rotating committee?

Yeah probably missing tests.

bkchr

Just some last nitpicks, otherwise looks good!

bkchr · 2025-12-08T22:12:29Z

 pub struct WarpSync<B: BlockT, Client> {
 	phase: Phase<B>,
-	client: Arc<Client>,
+	_client: Arc<Client>,


Let's remove it. We still need client in new. So, we don't need to change any interfaces, but we don't need to store it here.

bkchr · 2025-12-08T22:15:35Z

-				*authorities = new_authorities;
+			Ok(VerificationResult::Partial(new_last_hash, proofs)) => {
+				debug!(target: LOG_TARGET, "Verified partial proof");
 				*last_hash = new_last_hash;


Can we just remove last_hash completely and let verifier.context always return the latest hash. Maybe let's rename context to something like next_proof_context or something along these lines.

lexnv · 2025-12-09T16:51:33Z

 			})
 			.collect::<Vec<_>>();
+		{
+			let mut state = self.state.lock();


The state might become inconsistent, since there's a possibly large gap between the acquired locks. Is this a scenario that can happen within our code?

-T0: [thread 0] first .lock
-T1: [thread 1] first .lock
-T2: [thread 1] complets verifying and sets [next_auth 1, last_header 1]
-T3: [thread 0] complets verifying but now we are overwritting the sets set at T2

6e35423
Thank you, I made it scoped. My first thought was that this process is synchronous by default, introduction of Mutex/RefCell is for immutability

bkchr

Yeah last change and then we can merge this.

bkchr · 2025-12-12T08:05:50Z

-			proof.verify(set_id, authorities, &self.hard_forks).map_err(Box::new)?;
+
+		{
+			let mut state = self.state.lock();


Just let's make verify take &mut self. We don't really need the mutex here.

bkchr · 2025-12-12T08:08:05Z

Please also merge master, there are some fixes for some of the flaky CI jobs.

Klapeyron · 2025-12-12T09:13:22Z

Thank you. @bkchr if you could also take a look on this one: #10223 (comment), looks like there is some missing test for warp with rotating committee, due at the moment when this comment was created, validator was not persisting new authority set and CI was green. Maybe it can be addressed in some follow-up issue

lexnv

LGTM! Thanks for contributing 🙏

Klapeyron force-pushed the context_for_warp_sync_provider branch from d3b5b26 to 2ef715e Compare November 7, 2025 16:41

bkchr requested changes Dec 3, 2025

View reviewed changes

Klapeyron added 6 commits December 4, 2025 17:14

Added WarpSyncContext for WarpSyncProvider

10f560a

fixup! Added WarpSyncContext for WarpSyncProvider

d2b98b2

Revert "fixup! Added WarpSyncContext for WarpSyncProvider"

8126d8e

This reverts commit efba492.

Revert "Added WarpSyncContext for WarpSyncProvider"

840b671

This reverts commit 2ef715e.

Added Verifier for WarpSyncProvider

5f0d0a9

fixup! Added Verifier for WarpSyncProvider

7d5cc5c

Klapeyron force-pushed the context_for_warp_sync_provider branch from efba492 to 7d5cc5c Compare December 4, 2025 18:48

Klapeyron changed the title ~~Added WarpSyncContext for WarpSyncProvider~~ Added Validator to WarpSyncProvider Dec 4, 2025

Klapeyron commented Dec 4, 2025

View reviewed changes

fixup! Added Verifier for WarpSyncProvider

0d9a81c

bkchr approved these changes Dec 8, 2025

View reviewed changes

bkchr added the T0-node This PR/Issue is related to the topic “node”. label Dec 8, 2025

Klapeyron added 3 commits December 9, 2025 13:31

fixup! Added Verifier for WarpSyncProvider

81f2f51

fixup! Added Verifier for WarpSyncProvider

3127d06

fixup! Added Verifier for WarpSyncProvider

0de006f

lexnv reviewed Dec 9, 2025

View reviewed changes

Klapeyron added 2 commits December 9, 2025 18:57

fixup! Added Verifier for WarpSyncProvider

6e35423

fixup! Added Verifier for WarpSyncProvider

a2a0618

bkchr approved these changes Dec 12, 2025

View reviewed changes

bkchr requested a review from lexnv December 12, 2025 08:08

bkchr mentioned this pull request Dec 12, 2025

Improve Warp Sync Logging #10196

Merged

3 tasks

Klapeyron and others added 2 commits December 12, 2025 10:08

fixup! Added Verifier for WarpSyncProvider

d79ea6b

Merge branch 'master' into context_for_warp_sync_provider

612f3c1

fixup! fixup! Added Verifier for WarpSyncProvider

fa2447c

bkchr approved these changes Dec 15, 2025

View reviewed changes

bkchr enabled auto-merge December 15, 2025 10:00

lexnv approved these changes Dec 15, 2025

View reviewed changes

bkchr added this pull request to the merge queue Dec 15, 2025

Merged via the queue into paritytech:master with commit 310625d Dec 15, 2025
252 of 258 checks passed

pgherveou added this to [preview] release tracker Mar 31, 2026

github-project-automation Bot moved this to Done in [preview] release tracker Mar 31, 2026

Conversation

Klapeyron commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Klapeyron commented Dec 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr commented Dec 12, 2025

Uh oh!

Klapeyron commented Dec 12, 2025

Uh oh!

lexnv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Klapeyron commented Nov 6, 2025 •

edited

Loading